-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add method chaining API for query pagination #62
Add method chaining API for query pagination #62
Conversation
retrievePage(num) { | ||
this.page = num; | ||
return this; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akiroz I prefer keeping the function name as limit(num)
, offset(num)
and offset(num)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ben181231 but the names are already used by the fields, if I override them it will break backwards compatibility....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I see.
@akiroz Please attach the related documentation update PR to this PR. |
I think we may not want to add this method chaining to Query. Consider what does this mean:
The end result is skipping 2, 5 or 7 result? I think this API create ambiguity. The current query chaining interface only exist for modifying the internal mutable property. i.e. So far no setter method(chaining or not) on property that are immutable. In addition, one way to do a things is better than many ways for doing things. It reduces the pain on support. |
I think this pull request is incomplete, but I am open to the discussion of better support of chain result. I think the ambigiuty Rick mentioned can be solved by if some parameter such as limit or skip was called twice error will be throw. It is not something very new. But I also agree that if limit, skip, offset are chainable, people is reasonable to expect other query filter like equalTo are chainable too. Regarding 1 way to do things is better, I think it depends on how common this way of doing thing is in JavaScript. Which my impression is pretty common, so that's why I am open to that. But we probably need more planning on the API (how to keep backward compatiability or not?) and it should works amongst to whole set of query operator before merge into master |
Maybe it is a little bit off topic but I wrote myself a paginator to handle pagination with skygear.Query. Here is the code const generatePaginator = ({doQuery}) => ({limit}) => (queryProducer) => {
const query = queryProducer();
let offset = 0;
let isFetching = false;
let isEnded = false;
let allRecords = [];
const next = () => {
if (isFetching) {
return Promise.resolve();
}
if (isEnded) {
return Promise.resolve([]);
}
isFetching = true;
return doQuery(query, limit, offset)
.then(_records => {
const records = Array.prototype.slice.call(_records);
isFetching = false;
offset += records.length;
if (records.length < limit) {
isEnded = true;
}
if (records.length > 0) {
allRecords = allRecords.concat(records);
}
return records;
})
.catch(e => {
isFetching = false;
return Promise.reject(e);
});
};
return {
next,
get isEnded() {
return isEnded;
},
get isFetching() {
return isFetching;
},
get allRecords() {
return allRecords;
},
};
};
export default generatePaginator; It is generic so we need some glue code to adapt the skygear.Query interface const skygearPaginator = generatePaginator({
doQuery(query, limit, offset) {
query.limit = limit;
query.offset = offset;
return skygear.publicDB.query(query);
},
});
const paginatorWith50ItemsPerPage = skygearPaginator({limit: 50}); In some react component componentDidMount() {
this.paginator = paginatorWith50ItemsPerPage(() => {
const q = new skygear.Query(skygear.Record.extend('product'));
return q;
});
this.fetch();
}
fetch() {
this.paginator.next().then(records => this.setState({ records }));
}
onEndReach() {
// some callback fired by some ListView-like component
this.fetch();
} So you don't have to manage offset by yourself. |
@rickmak I don't understand the problem.... |
@akiroz There are no |
@cheungpat Ultimately, I want all calls to be chain-able when I construct my record objects because otherwise people will start writing their own factory functions for building records. what do you think? |
You are extending the interface, @rickmak mentions that the new interface is ambiguous. You are trying to address the concern by comparing the interface you extended with a hypothetical interface that is based on an interface you extended. The difference is irrelevant. You still have to address the ambiguity introduced by your extension. I do not have a vested interest in how the interface should work, but |
I am closing this if no more update. |
No description provided.